-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding LITTLEFS w/o rebuild the IDF #4096
Conversation
based on https://github.com/joltwallet/esp_littlefs This will be my first PR, hope will do it right.
Anyway, the LITTLEFS is a hack.
Anyway, the LITTLEFS is a hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is embedding https://github.com/joltwallet/esp_littlefs it would be good to have sign-off from @joltwallet / @BrianPugh since some files are showing mixed/missing licensing.
This PR will also need to be updated for the esp32s2 branch which is based on IDF v4.2 (master) as there are VFS API changes which may break this functionality.
} | ||
] | ||
} | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reformatting of an existing file (such as this one) is often discouraged and will often lead to the PR sitting for a very long time (if it ever gets merged).
If you do reformat the file, please ensure it is consistent as it doesn't appear to be consist after reformatting (looks like a mix of spaces and tabs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joltwallet and @BrianPugh
Could you please help in this?
libraries/LITTLEFS/src/LITTLEFS.cpp
Outdated
//esp_err_t esp_littlefs_format(const char* partition_label); | ||
//esp_err_t esp_littlefs_info(const char* partition_label, size_t *total_bytes, size_t *used_bytes); | ||
|
||
#define LFS_NAME "spiffs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of a hardcode via DEFINE can you try:
static constexpr const char LFS_NAME[] = "spiffs";
This will prevent global namespace pollution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
CMakeLists.txt
Outdated
libraries/LITTLEFS/src/esp_littlefs.c | ||
libraries/LITTLEFS/src/lfs.c | ||
libraries/LITTLEFS/src/lfs_util.c | ||
libraries/LITTLEFS/src/littlefs_api.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please correct formatting on these additions.
libraries/LITTLEFS/src/LITTLEFS.cpp
Outdated
|
||
using namespace fs; | ||
|
||
class LITTLEFSImpl : public VFSImpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@me-no-dev would it be feasible to provide a default impl for VFSImpl that can be used by the various FS impls rather than each one copying/pasting the same but with a different name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, this class can be dropped entirely.
libraries/LITTLEFS/src/LITTLEFS.cpp
Outdated
@@ -0,0 +1,130 @@ | |||
// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjust copyright year to 2020 (for all files with similar header)
Asking Brian to review...
Someone else should help here please to adapt the code to match older IDF, maybe @BrianPugh is the best to help with such branch of his library. |
Hey guys, Some questions/feedback:
My action plans:
edit 1: Added MIT license to my repo |
IDF v4.0 changed a few things: https://github.com/espressif/esp-idf/releases/tag/v4.0, there are likely more changes beyond v4.0 that would need to be handled. You can detect the IDF version via a few compile time defines. |
ah gotcha, I've been regularly using my repo off of esp-idf master for esp32 targets, so it should be good to go. I thought there may be even newer or esp32-s2 specific changes. |
Great! I don't know of any changes specific to the ESP32-S2 that will prevent this from working. |
@BrianPugh Hi Brian, It will be difficult for me to re-wrap the LITTLEFS.cpp and .h in such a way that your library is kept as it is and just fork the appropriate revision of it. No problem if you rewrite it entirely around your wrap, especially if it can be brought back to Arduino w/o painful recompiling and IDF syncing. LL |
The best way to avoid that would be have @BrianPugh submit a PR to ESP-IDF to have LittleFS included as a component there. Then it can be wrapped in arduino-esp32 in a clean manner (like other FS types) |
I'll make a PR, no harm in trying. I fear that there might be a lot of other requirements (writing up a lot of documentation, etc) |
I wish you a patience and good luck with that :) |
Here's a link to my PR for littlefs in esp-idf: |
If you've used the package, you know documentation is not always most thorough... |
For registering, is that targeted at me or @lorol ? I don't really use arduino (and am unfamiliar with its package managers, library structures, and all that). I'm just here to help on actions requiring changes in my repo. |
Targeted at OP (@lorol), whom wants it in arduino-esp32. |
libraries/LITTLEFS/src/LITTLEFS.cpp
Outdated
|
||
using namespace fs; | ||
|
||
class LITTLEFSImpl : public VFSImpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, this class can be dropped entirely.
libraries/LITTLEFS/src/LITTLEFS.cpp
Outdated
return (f == true) && !f.isDirectory(); | ||
} | ||
|
||
LITTLEFSFS::LITTLEFSFS() : FS(FSImplPtr(new LITTLEFSImpl())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to:
LITTLEFSFS::LITTLEFSFS() : FS(FSImplPtr(new VFSImpl()))
and remove the definition of class LITTLEFSImpl. The customization is not necessary for LITTLEFS as the default behavior from VFSImpl() will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thanks but ,,, |
#define CONFIG_LITTLEFS_LOOKAHEAD_SIZE 128
- In esp_littlefs.c, added a way to build for older IDF too (IDF 3.2 is used on release 1.0.4) #define CONFIG_LITTLEFS_FOR_IDF_3_2
Modified Update library to allow FatFS OTA Updated file: package_esp32_index.template.json Added a sub-folder with IDE plugins
Updated esp32fs.jar and README for it
Request done: arduino/Arduino#10719 |
Here is how we will go for 2.0.0:
|
@lorol esp_littlefs has been added to the esp32s2 branch. You can try it there. |
Ok, great! I see the lib.a and headers. Will try. I'll let you or Brian know if I need help to make it correct. |
@me-no-dev hardware\espressif\esp32/tools/sdk/esp32/include/newlib/platform_include/sys/termios.h:97: warning: "B110" redefined hardware\espressif\esp32\cores\esp32/binary.h:65: note: this is the location of the previous definition hardware\espressif\esp32/tools/sdk/esp32/include/driver/include/driver/adc_common.h:368:11: note: declared here Actually it builds w/o adding lfs files as submodule. I guess already built in through esp_littlefs? |
@lorol lfs is already in. you only need the two LITTLEFS source files :) to fix the redefine warning, I need the full error trace, so we can see where they are both defined. ADC I'll fix later. |
@lorol I'll suggest you open a pull request to the esp32s2 branch so we can give it a go (will need to add the tools, etc as well) |
@me-no-dev @BrianPugh In file included from --\hardware\espressif\esp32/tools/sdk/esp32/include/esp_littlefs/include/esp_littlefs.h:16, In file included from --\hardware\espressif\esp32\cores\esp32/Arduino.h:40, In file included from --\hardware\espressif\esp32/tools/sdk/esp32/include/esp_littlefs/include/esp_littlefs.h:16, In file included from --\hardware\espressif\esp32\cores\esp32/Arduino.h:40, Also you need to apply: I will cleanup the LITTLEFS examples and try to do a PR at esp32s2 branch (with some hacky tools addition proposal to work with this https://github.com/lorol/arduino-esp32fs-plugin - of course you may need to do it in better way) Observed huge RAM heap difference of my sketch between latest esp32s2 and master branches - about 70K more taken on s2. Board is TTGO T1 |
@lorol I don't believe we need |
@lorol I am aware of the ~70k issue. Need to figure out where those bytes went. I also agree with @BrianPugh |
@lorol rmdir fix is applied. pull the changes |
closing now :) |
based on https://github.com/joltwallet/esp_littlefs
This will be my first PR, hope will do it right.
Details in README